sima0_16_001: Make constituent names case-insensitive in IC file read path, consistent with the CCPP framework; update framework tag#518
Conversation
…allback Cherry-picks the case-insensitive standard-name matching from e4327dd and extends the constituent ic-read fallback to try all-upper and all-lower case spellings. Standard names are case-insensitive -- capgen lowercases scheme-side names (e.g. 'co2') while registry declarations keep their authored case ('CO2', 'O3', ...) -- but the ic-read glue compared them case-sensitively, and the not-in-registry snapshot fallback built candidate netCDF names only in the runtime (lowercase) case. netCDF variable names are case-sensitive, so a registry constituent declared with authored case, or an uppercase snapshot field ('CO2', 'cnst_CO2'), silently failed to match and fell back to zeros. - physics_data.F90: find_input_name_idx and the constituent-dimensioned shortmap loop compare case-insensitively; a new helper builds constituent-dimensioned file names in as-registered/upper/lower spellings. - write_init_files.py: generated phys_read_data/phys_check_data compare case-insensitively, and the not-in-registry fallback now tries {std_name, cnst_<>, pbuf_<>} x {as-is, upper, lower}. - cam_var_init_marks.inc: mark_as_*/is_* lookups compare case-insensitively. Preserves 0d6d482's std_name_len+5 sizing (a raw cherry-pick of e4327dd would have regressed it). Regenerated write_init_files sample fixtures; write_init_files (16/16) and registry (29/29) python unit tests pass.
GitHub Actions
Inactive
gather_ccpp_req_vars added any constituent not listed in registry.xml to the required host-variable list, which gave it a phys_var_stdnames row keyed to the scheme's local Fortran variable name (e.g. 'co2mmr' for CO2 in modal_aero_setsox). At runtime that phantom row matched (const_input_idx>0), so the constituent was read by its local name -- which never matches a CAM snapshot field (constituents are written as <CNST_NAME>/cnst_<CNST_NAME>, uppercase, and netCDF names are case-sensitive) -- pre-empting the cnst_/pbuf_ standard-name fallback and silently leaving the constituent at its default (the setsox xco2=0 symptom). Only advected constituents are suppressed from in_vars; constituent-related variables that are not advected (e.g. tendency variables like tendency_of_water_vapor_mixing_ratio_wrt_moist_air_and_condensed_water) are still added as regular host variables -- they are read via the host variable path, not the constituent loop. The now-unused registry_constituents argument to gather_ccpp_req_vars is dropped and write_ic_arrays' registry skip is restored to its original form. Sample fixtures unchanged; write_init_files (16/16) and registry (29/29) pass.
3cd5c84 to
4ec5892
Compare
GitHub Actions
Inactive
peverwhee
left a comment
peverwhee
left a comment
There was a problem hiding this comment.
One question.
Also: I'm hopeful that once we get rid of build-time constituents and everything is runtime, we'll be able to use the constituent diag_name to ease some of this logic.
GitHub Actions
Error
GitHub Actions
Failure
peverwhee
left a comment
peverwhee
left a comment
There was a problem hiding this comment.
thanks @jimmielin !
The new framework tag is available for you: sima_2026-07-15
Co-authored-by: Courtney Peverley <courtneyp@ucar.edu>
GitHub Actions
Inactive
|
Thanks @peverwhee! Regression tests started! Wish me luck! |
GitHub Actions
Failure
GitHub Actions
Failure
jimmielin
changed the title
|
It looks like the regression tests passed, but CIRRUS won't work so I will end up running nvhpc instead. |
GitHub Actions
Failure
GitHub Actions
Failure
GitHub Actions
Inactive
nusbaume
left a comment
nusbaume
left a comment
There was a problem hiding this comment.
Thanks @jimmielin! I have a few requests, mostly related to what looks like the AI being overly-verbose, but nothing that is strictly required or needs a re-review.
| if cvar.get_prop_value('advected'): | ||
| # Advected constituents are allocated/registered by the | ||
| # constituents object and read via the runtime | ||
| # constituent loop (registry ic_file_input_names or | ||
| # the cnst_/pbuf_ fallback). Do NOT add them to the | ||
| # host-variable list: that gives them a | ||
| # phys_var_stdnames row keyed to the scheme's local | ||
| # name, which never matches a CAM snapshot field and | ||
| # would pre-empt the fallback. | ||
| pass | ||
| else: |
There was a problem hiding this comment.
This feels like a situation where the AI is over-explaining something (i.e. most of time a human wouldn't write a paragraph explaining the no-op path of a conditional). Instead I might just change the conditional to its inverse and remove this pass section, especially since the else comment seems to explain it well enough.
| if cvar.get_prop_value('advected'): | |
| # Advected constituents are allocated/registered by the | |
| # constituents object and read via the runtime | |
| # constituent loop (registry ic_file_input_names or | |
| # the cnst_/pbuf_ fallback). Do NOT add them to the | |
| # host-variable list: that gives them a | |
| # phys_var_stdnames row keyed to the scheme's local | |
| # name, which never matches a CAM snapshot field and | |
| # would pre-empt the fallback. | |
| pass | |
| else: | |
| if not cvar.get_prop_value('advected'): |
Of course if you disagree just let me know, as I could certainly be convinced otherwise!
There was a problem hiding this comment.
Thanks! I agree this is overly verbose but I think we could document the if not clause with a reason. Would this be better?
# Do not add advected constituents to the host variable
# list so they are not included in phys_var_stdnames and
# can be read in the constituent path:
if not cvar.get_prop_value('advected'):There was a problem hiding this comment.
Yeah that comment block for the if not clause looks good to me, so feel free to add it. Thanks!
| # The <std_name>, cnst_<std_name>, pbuf_<std_name> prefix default fallbacks -- | ||
| # in the as-registered, all-upper, and all-lower case spellings -- allow us to | ||
| # not enumerate all needed constituents from snapshots in the registry, yet | ||
| # allow reading their values from CAM snapshots regardless of authored name case. |
There was a problem hiding this comment.
I might get rid of this comment block, as the text in the generated comments explains it well enough, at least in my opinion.
There was a problem hiding this comment.
I also don't like this, so I am happy to remove. Would you think it's self-explanatory that some constituents in CAM are uppercase and some lowercase? I briefly considered changing this to something like
# Try uppercase and lowercase variants for constituent names to match CAM
# e.g., O3, NO2, NUMLIQ (most species) ... num_a1, num_a2 (aerosols)
There was a problem hiding this comment.
I personally think it is self-explanatory, simply because having a system that tries the different case combinations is inherently more robust than a system that doesn't cover that space, regardless of what case(s) the constituents actually have.
That being said if you don't feel the same then I am happy to include the comment you are proposing (as it is still way shorter than what was originally there). Thanks!
| # The <std_name>, cnst_<std_name>, pbuf_<std_name> prefix default fallbacks -- | ||
| # in the as-registered, all-upper, and all-lower case spellings -- allow us to | ||
| # not enumerate all needed constituents from snapshots in the registry, yet | ||
| # allow reading their values from CAM snapshots regardless of authored name case. |
There was a problem hiding this comment.
I would remove this comment-block as well.
There was a problem hiding this comment.
Please see question above!
|
|
||
| character(len=*), intent(in) :: base_name | ||
| character(len=*), intent(in) :: cname | ||
| character(len=256) :: var_names(3) |
There was a problem hiding this comment.
Since this is SIMA-specific code we can use the shared CESM character lengths:
| character(len=256) :: var_names(3) | |
| use shr_kind_mod, only: cl=>shr_kind_cl | |
| ... | |
| character(len=cl) :: var_names(3) |
GitHub Actions
Inactive
jimmielin
added
AI-generated code
BFB
|
I ran the regression tests last night and it looks like trcdata_bam is failing because it uses the legacy |
|
Needs the fixes from ESCOMP/atmospheric_physics#424 then this will be ready to merge - bfb. |
… fully use diagnostic names in sima_state_diagnostics (#424) Tag name (The PR title should also include the tag name): Originator(s): @jimmielin Description (include issue title and the keyword ['closes', 'fixes', 'resolves'] and issue number): - Change prescribed chemistry/aerosol schemes to use ccpp_scheme_utils so they are case-insensitive - companion PR to ESCOMP/CAM-SIMA#518 - #397 (in 979c6bc) changed `sima_state_diagnostics` to use `%diagnostic_name` to add field but not the write loop, so if the two cases mismatched (due to the case-insensitivity fixes they now do) the `history_out_field` would be mismatched and would write uninitialized heap memory into the netCDF file rather than the variable. List all namelist files that were added or changed: List all files eliminated and why: List all files added and what they do: List all existing files that have been modified, and describe the changes: (Helpful git command: `git diff --name-status main...<your_branch_name>`) ``` M schemes/chemistry/prescribed_aerosols.F90 M schemes/chemistry/prescribed_aerosols.meta M schemes/chemistry/prescribed_ozone.F90 M schemes/chemistry/prescribed_ozone.meta - change to use ccpp_scheme_utils. M schemes/sima_diagnostics/sima_state_diagnostics.F90 - change to use diagnostic_name in history_out_field. ``` List all automated tests that failed, as well as an explanation for why they weren't fixed: Is this an answer-changing PR? If so, is it a new physics package, algorithm change, tuning change, etc? If yes to the above question, describe how this code was validated with the new/modified features: --------- Co-authored-by: Jesse Nusbaumer <nusbaume@ucar.edu>
jimmielin
changed the title
|
This is not answer-changing but the GNU test ran to completion, so I am giving it a tag nubmer to archive the baseline. |
. ESCOMP#518 stopped gather_ccpp_req_vars adding constituents to the required host-variable list, which drops 'band_number' from the bad-vertical-dimension sample (phys_var_num 3 -> 2, ic_name_len 7 -> 5), but the committed sample file was not regenerated. test_write_init_files::test_bad_vertical_dimension fails on development as a result. Regenerated output is byte-identical to what development's own write_init_files.py produces, so this applies to development standalone.
The constituent read/check paths are now generated by development's ESCOMP#518 logic (case-insensitive standard-name matching plus the all-case cnst_/pbuf_ candidate list) rather than the superseded branch-local version that was dropped in this rebase.
3 participants
Tag name (required for release branches):
Originator(s): @jimmielin
AI tools used (if applicable; please also add the "AI-generated code" label to the PR):
What: see below
How: claude-fable:5 identified and drafted the original case-insensitive fix; claude-opus:4.8 refined the fix
Description (include the issue title, and the keyword ['closes', 'fixes', 'resolves'] followed by the issue number):
ic_file_input_namematching. Standard names are case-insensitive and capgen lowercases scheme-side names (e.g.co2), but the IC read code compared them case-sensitively so a registry constituent with authored case (CO2,O3) or an uppercase snapshot field read nothing (could see this failure inatm.log...it falls back to the argument name). All constituent-name comparisons are now case-insensitive.{std_name, cnst_, pbuf_} × {as-is, UPPER, lower}so a lowercase runtime name (co2) still matches an uppercase snapshot field (CO2,cnst_CO2). netCDF variable names are case-sensitive. CAM has uppercase for most chemistry and physics fields and lowercase for aerosols (e.g.,num_a1)gather_ccpp_req_varsno longer adds constituents to the required host-variable list, which previously gave them aphys_var_stdnamesrow keyed to the scheme's local Fortran name (e.g.co2mmr) that preempted the cnst_/pbuf_ fallback and left the constituent at its default.Describe any changes made to build system: N/A
Describe any changes made to the namelist: N/A
List any changes to the defaults for the input datasets (e.g. boundary datasets):
List all files eliminated and why:
List all files added and what they do:
List all existing files that have been modified, and describe the changes:
(Helpful git command:
git diff --name-status development...<your_branch_name>)If there are new failures (compared to the
test/existing-test-failures.txtfile),have them OK'd by the gatekeeper, note them here, and add them to the file.
If there are baseline differences, include the test and the reason for the
diff. What is the nature of the change? Roundoff?
derecho/intel/aux_sima:
SMS_Ln9.ne3pg3_ne3pg3_mg37.FKESSLER.derecho_intel.cam-outfrq_se_cslam_multitape (Overall: NLFAIL) details:
FAIL SMS_Ln9.ne3pg3_ne3pg3_mg37.FKESSLER.derecho_intel.cam-outfrq_se_cslam_multitape NLCOMP
Pre-existing failure
derecho/gnu/aux_sima:
SMS_Ln9.ne3pg3_ne3pg3_mg37.FADIAB.derecho_gnu.cam-outfrq_se_cslam (Overall: DIFF) details:
FAIL SMS_Ln9.ne3pg3_ne3pg3_mg37.FADIAB.derecho_gnu.cam-outfrq_se_cslam BASELINE /glade/campaign/cesm/community/amwg/sima_baselines/latest_gnu: ERROR BFAIL some baseline files were missing
Suspect test
derecho/nvhpc/aux_sima (test is run via Github workflow. Only run the test manually if we need to save new baselines): All PASS
If this changes climate describe any run(s) done to evaluate the new
climate in enough detail that it(they) could be reproduced:
CAM-SIMA date used for the baseline comparison tests if different than latest: